[js-sdk-release-tools] remove existing node_modules before restoring from backup#13308
Conversation
…from backup otherwise the follow error could be thrown when the directory exists >[ERROR]: ENOTEMPTY: directory not empty, rename '/mnt/vss/_work/1/s/azure-sdk-for-js/node_modules_backup' -> '/mnt/vss/_work/1/s/azure-sdk-for-js/node_modules'
There was a problem hiding this comment.
Pull request overview
This PR addresses a file system error that occurs during the node_modules restoration process when the target directory already exists. The solution removes any existing node_modules directory before attempting to rename the backup.
Key changes:
- Renamed variable from
nodeModulesPathtonodeModulesBackupPathfor clarity - Added logic to remove existing node_modules directory before restoration
- Split the restoration logic into separate existence checks for current and backup directories
Comments suppressed due to low confidence (1)
tools/js-sdk-release-tools/src/utils/backupNodeModules.ts:28
- The restoreNodeModules function, which includes critical file system operations (recursive deletion and renaming), lacks test coverage. The repository has comprehensive test coverage in the src/test directory for other utilities. Consider adding tests to verify the restore behavior, especially edge cases like when backup exists, when it doesn't exist, and when both backup and current node_modules exist.
export async function restoreNodeModules(folder: string) {
const nodeModulesBackupPath = path.join(folder, "node_modules_backup");
const nodeModulesPath = nodeModulesBackupPath.replace('_backup', '');
if (fs.existsSync(nodeModulesPath)) {
logger.info(`Remove existing '${nodeModulesPath}' first.`);
fs.rmSync(nodeModulesPath, { recursive: true, force: true });
}
if (fs.existsSync(nodeModulesBackupPath)) {
logger.info(`Start to rename '${nodeModulesBackupPath}' to '${nodeModulesPath}'.`);
fs.renameSync(nodeModulesBackupPath, `${nodeModulesPath}`);
}
if ('/' === path.dirname(folder)) return;
await restoreNodeModules(path.dirname(folder));
}
|
cc @skywing918 I am not sure whether it is expected to assume |
MaryGao
left a comment
MaryGao
left a comment
There was a problem hiding this comment.
I remember we met some issues but I can't remember details and that's why we did the restore things.
@skywing918 Could you verify if we would meet issues with this change?
|
I don't think we can remove this part as it will cause some automation issue. Please double confirm. |
|
base on #5276, seems also I have submit some jobs with this change. all jobs passed. |
|
The original issue is probably related to rush or some other tool removing |
|
Are there any other concerns for this PR? or should we remove the backup and restore entirely? |
MaryGao
left a comment
MaryGao
left a comment
There was a problem hiding this comment.
LGTM since we verified there is no issue.
otherwise the follow error could be thrown when the directory exists